/jira

💫 Jira ticket created: SRVKP-8026

🤖 AI Review provided by gemini:gemini-2.5-pro:

Excellent! This is a substantial and well-thought-out feature commit. It adds a powerful new tkn pac cel command with great features like provider auto-detection and an interactive shell. The test coverage is also impressive.

Here is a careful analysis of the changes, highlighting potential bugs, spelling mistakes, and code improvements.

Summary of Analysis

Overall, this is a high-quality commit. The core logic is solid, and the feature is well-implemented with good test coverage. The identified issues are relatively minor and easy to address.

• Bugs: A couple of minor bugs were found, mainly related to redundant code and case-sensitive handling of HTTP headers.
• Spelling: A few minor typos in comments and test data.
• Improvements: Suggestions to make the code more robust and idiomatic.

🐛 Bugs & Potential Issues

These are items that could cause incorrect behavior under certain conditions.

1. Redundant JSON Unmarshaling

   In the functions that parse provider payloads, json.Unmarshal is called on the payload body after the provider's SDK has already parsed it. This is unnecessary and can be removed for cleaner code.

   • File: pkg/cmd/tknpac/cel/cel.go
   • In eventFromGitHub(): The line _ = json.Unmarshal(body, &ghEvent) is redundant because github.ParseWebHook already unmarshals the data.
   • In eventFromGitLab(): The block if err := json.Unmarshal(body, &eventInt); err != nil { ... } is redundant because gitlab.ParseWebhook already handles this.

   Recommendation: Remove these redundant json.Unmarshal calls. The preceding ParseWebhook functions have already done the work.

2. Case-Sensitive GitHub Header Lookup

   HTTP headers are defined as case-insensitive. While most of the new code handles this correctly (e.g., using strings.EqualFold), the GitHub event parsing does not.

   • File: pkg/cmd/tknpac/cel/cel.go
   • In eventFromGitHub(): The line event.EventType = headers["X-GitHub-Event"] performs a case-sensitive lookup. If a user provides headers with x-github-event, this will fail to find the event type.

   Recommendation: Make the lookup case-insensitive to be more robust. You can check for both casings, as is done in the detectProvider function:

   // In eventFromGitHub()
   event.EventType = headers["X-GitHub-Event"]
   if event.EventType == "" {
       event.EventType = headers["x-github-event"]
   }

📝 Spelling and Documentation

These are minor typos and clarifications in comments and documentation.

1. Typo in Test Error String

   There's a casing typo in an expected error message within the tests.

   • File: pkg/cmd/tknpac/cel/cel_test.go
   • In TestCommandExecution and TestCommandFileHandling: The expectation wantErrContains: "unknown X-Github-Event" should be wantErrContains: "unknown X-GitHub-Event".

2. Misleading Comment

   A comment in the main command logic is slightly misleading.

   • File: pkg/cmd/tknpac/cel/cel.go, in Command() function.
   • Comment: // Create files data structure (empty for now since we don't have a provider)
   • Issue: At this point in the code, the provider has been determined. The reason the files data is empty is because file change detection is not implemented in the CLI.
   • Recommendation: Change the comment to be more accurate, for example: // Create files data structure (always empty in CLI mode).

3. Minor Typo in Documentation

   • File: docs/content/docs/guide/cli.md
   • Text: "...content of the webhook requests..."
   • Recommendation: Change to "...content of the webhook request...".

💡 Code Improvements & Idiomatic Go

These are suggestions to make the code cleaner, more robust, or more idiomatic.

1. Refactor Bitbucket Provider Detection Logic

   The logic to distinguish Bitbucket Cloud from Data Center in detectProvider works, but it contains a slightly strange-looking repeated if block. It can be made more concise.

   • File: pkg/cmd/tknpac/cel/cel.go
   • Original Code:

     // ...
     if actor, ok := payload["actor"].(map[string]any); ok {
         if _, hasAccountID := actor["account_id"]; hasAccountID {
             return "bitbucket-cloud", nil
         }
     }
     // Bitbucket Data Center has different structure
     if actor, ok := payload["actor"].(map[string]any); ok {
         if _, hasID := actor["id"]; hasID {
             return "bitbucket-datacenter", nil
         }
     }
     // ...

   • Recommendation: Combine these into a single if block for better readability.

     if actor, ok := payload["actor"].(map[string]any); ok {
         if _, hasAccountID := actor["account_id"]; hasAccountID {
             return "bitbucket-cloud", nil
         }
         // Heuristic: if it has an `id` but not an `account_id`, assume it's Data Center.
         if _, hasID := actor["id"]; hasID {
             return "bitbucket-datacenter", nil
         }
     }

2. Explicitly State files.* Limitation in Help Template

   The help template is very good, but the note about files.* functionality could be slightly punchier.

   • File: pkg/cmd/tknpac/cel/templates/help.tmpl
   • Suggestion: The current text is clear but a bit long. Consider combining the two lines about file variables.

     -  📁 Files (files.* - ❌ always empty in CLI):
     -    • files.all, files.added, files.modified, etc.
     -    ⚠️  Note: fileChanged, fileDeleted, fileModified and all files.* variables are not implemented in the CLI and will always be empty.
     +  📁 Files (files.*):
     +    ⚠️  Note: File-related variables (e.g., `files.all`) and functions (e.g., `fileChanged`) are not supported in the CLI and will be empty or false.

   This is a stylistic choice; the original is also perfectly fine.

Final Verdict

This is an excellent commit that adds a very useful feature. The implementation is robust, especially with the fallback from readline to survey and the thoughtful provider auto-detection logic. The identified issues are minor and do not detract from the overall quality of the contribution. Congratulations to the author on a job well done

/retest

/lgtm

/retest

it is LGTM, do I merge or you wanna make more changes?

@zakisk i think wait for it, there is some edge case that makes it buggy compared to live event... i'll let you know if we keep it "as is"

/gemini review